Make loaned messages const on the subscription side#1971
Make loaned messages const on the subscription side#1971nnmm wants to merge 2 commits intoros2:rollingfrom
Conversation
Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai>
…sage Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai>
ivanpauno
left a comment
ivanpauno
left a comment
There was a problem hiding this comment.
It would be nice to see some test cases before we merge this.
| typename std::remove_const<MaybeConstROSMessageType>::type | ||
| >::value | ||
| >::type | ||
| dispatch( |
There was a problem hiding this comment.
could you add tests making sure that loaned messages work well with all types of callbacks?
There're a lot of codepaths here, and it's pretty hard to make sure you didn't break anything else.
So it would be nice to make sure all cases are covered in tests (some of the cases might already be covered).
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with test cases mentioned https://github.com/ros2/rclcpp/pull/1971/files#r919331175
|
I've picked this up again – sorry for the delay. Are you fine if I make it an error to dispatch a loaned message to a callback that would require making a copy of the loaned message? Because I think that making a copy would in most cases not be intended. But type adaptation should probably be allowed. |
I'm not sure. @wjwwood what do you think? |
4 participants
Related to ros2/rcl#992.
To make callback dispatching work, I generalized
dispatch()to work with both const and non-const messages. Maybe there is a better way.